Skip to content

Conversation

@ErkoRisthein
Copy link
Contributor

Summary

Fixes TypeError: Cannot read properties of undefined (reading 'startsWith') that occurs when the window receives postMessage events that have a data property but no action property.

Root Cause

The condition event.data?.action.startsWith("web-eid:") safely handles when event.data is undefined, but throws when event.data exists and event.data.action is undefined.

Fix

Add optional chaining on action:

if (!event.data?.action?.startsWith("web-eid:")) return;

Test Plan

  • Added unit tests for messages with missing action property
  • npm test passes (48 tests)

Signed-off-by: Erko Risthein [email protected]

Copilot AI review requested due to automatic review settings December 11, 2025 10:14
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a TypeError that occurs when the window receives postMessage events with a data property but without an action property. The fix adds optional chaining to safely access the action property before calling startsWith().

Key Changes:

  • Added optional chaining operator to prevent TypeError when event.data.action is undefined
  • Added unit tests to verify messages without the action property are properly ignored

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/services/WebExtensionService.ts Added optional chaining to event.data.action to prevent TypeError when action is undefined
src/services/tests/WebExtensionService-test.ts Added three test cases verifying that messages with missing/null/undefined data or action properties are properly ignored

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@taneltm
Copy link
Member

taneltm commented Jan 5, 2026

Thanks for noticing the issue!

Made this mistake when I changed:

    if (!/^web-eid:/.test(event.data?.action)) return;

to

    if (!event.data?.action.startsWith("web-eid:")) return;

Your change does fix the issue, but only if we expect event.data.action to always be a string.
If a website or extension had action as an object, it would still cause an error.
For example:

window.postMessage(
    {
        action: {
            id: "<actionId>",
            _t: "<actionTimestamp>",
        },
        payload: {
            ...
        }
    },
    "*"
);

We could add another optional chaining check, so ...action?.startsWith?.("web-eid:"), but if we don't check the action type nor startsWith type, we could still end up with an error - Uncaught TypeError: event.data.action.startsWith is not a function.
For example, if someone uses the startsWith as a property name.

window.postMessage(
    {
        action: {
            startsWith: "2022-10-12",
            endsWith: "2026-10-12
        }
    },
    "*"
);

Adding type checks would make this IF statement harder to read at a glance, so I think the best option would be to revert it back to:

    if (!/^web-eid:/.test(event.data?.action)) return;

The regex .test(...) accepts any data type, but only matches strings.

What do you think, @ErkoRisthein ?

@ErkoRisthein
Copy link
Contributor Author

Thanks for the review, @taneltm. You're right that the optional chaining approach doesn't handle all edge cases.

I tried reverting to the regex approach you suggested:

if (!/^web-eid:/.test(event.data?.action)) return;

However, I discovered a non-intuitive edge case: JavaScript's RegExp.test() coerces its argument to a string before matching. This means an array like ["web-eid:test"] gets coerced to "web-eid:test" and passes the regex check. The code then fails later at message.action.endsWith() since arrays don't have that method.

So I've added an explicit type guard before calling startsWith:

if (typeof event.data?.action !== "string") return;
if (!event.data.action.startsWith("web-eid:")) return;

This handles all edge cases:

  • Objects like { id: "123" } are rejected by type check
  • Objects with startsWith property like { startsWith: "2022-10-12" } are rejected by type check
  • Arrays like ["web-eid:test"] are rejected by type check
  • Numbers, null, undefined are all rejected by type check

I've added test cases for these scenarios. All tests pass.

Copy link
Member

@taneltm taneltm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Original issue reproduced in main
  • PR fixes the issue
  • Confirmed that the tests are working properly (tests fail, as expected, when the fix is reverted)

@mrts
Copy link
Member

mrts commented Jan 6, 2026

@ErkoRisthein, heartfelt thanks for your contribution!

A couple of formalities still remain, then I will gladly merge this: can you please squash the commits into one, sign the Certificate of Origin with git commit --amend --signoff and do a force push? Thanks in advance!

@ErkoRisthein ErkoRisthein force-pushed the fix/null-safety-in-receive branch from 48a5149 to 62a703d Compare January 12, 2026 09:52
- Add typeof check to reject non-string action values early
- Use startsWith() safely after confirming action is a string
- Add test cases for various edge cases (object, number, array)

Signed-off-by: Erko Risthein <[email protected]>
@ErkoRisthein ErkoRisthein force-pushed the fix/null-safety-in-receive branch from 62a703d to 58f85d8 Compare January 12, 2026 09:55
@mrts mrts merged commit ee5b706 into web-eid:main Jan 12, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants